Fix invalid schools in our database#867
Merged
Merged
Conversation
We've added validation that makes existing schools we already have in our database. This means that that updates to those schools may fail and cause errors. Generally, when we add validation we should make sure it doesn't invalidate existing records. Here I've done two things: + Add `on: :create` to the new validations where schools have some invalid records + Add `unless: :rejected` to items with a unique condition where new signups may overlap with rejected schools. I've documented the errors in this sheet[1]. Some of the optional invalid codes could be fixed manually by nulling them if they aren't required, but since that can't be done for the required codes, I've handled them all the same way. https://docs.google.com/spreadsheets/d/1ZwkCrRkVeUcgUaDuDxSZljlhntmK1Az8J5--Y0bSBrI/edit?gid=0#gid=0
Test coverage91.57% line coverage reported by SimpleCov. |
Contributor
There was a problem hiding this comment.
Pull request overview
Adjusts School model validations to avoid invalidating existing schools records already present in the database, while updating model specs to reflect the intended “validate-on-create” behavior and rejected-record uniqueness rules.
Changes:
- Scope several validations to
on: :createto prevent updates failing for legacy invalid data. - Adjust uniqueness behavior around rejected schools (and add a spec for creator_id reuse).
- Update validation specs to explicitly assert behavior for new records.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
app/models/school.rb |
Changes validation contexts (on: :create) and adds unless: :rejected? to avoid conflicts with rejected schools. |
spec/models/school_spec.rb |
Updates/clarifies validation examples and adds coverage for creator_id reuse when the original school is rejected. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
abcampo-iry
approved these changes
Jun 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Status
What's changed?
We've added validation that invalidates existing schools we already have in our database. This means that that updates to those schools may fail and cause errors.
Generally, when we add validation we should make sure it doesn't invalidate existing records.
Here I've done two things:
on: :createto the new validations where schools have some invalid recordsunless: :rejectedto items with a unique condition where new signups may overlap with rejected schools.Using
on: :createis safe to do as we don't allow users to update these values. If we wanted to do this in the future we could provide a more complex condition (e.g. if a new record or that field has changed)I've documented the errors in this sheet. Some of the optional invalid codes could be fixed manually by nulling them if they aren't required, but since that can't be done for the required codes, I've handled them all the same way.
After deploy
Confirm all schools are valid by running
School.all.reject{|x| x.valid?}.count